Skip to content

tests: Require run-fail ui tests to have an exit code (SIGABRT not ok) #143002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 20, 2025

Conversation

Enselic
Copy link
Member

@Enselic Enselic commented Jun 25, 2025

And introduce two new directives for ui tests:

  • run-crash
  • run-fail-or-crash

Normally a run-fail ui test like tests that panic shall not be terminated by a signal like SIGABRT. So begin having that as a hard requirement.

Some of our current tests do terminate by a signal/crash however. Introduce and use run-crash for those tests. Note that Windows crashes are not handled by signals but by certain high bits set on the process exit code. Example exit code for crash on Windows: 0xc000001d (STATUS_ILLEGAL_INSTRUCTION). Because of this, we define "crash" on all platforms as "not exit with success and not exit with a regular failure code in the range 1..=127".

Some tests behave differently on different targets:

  • Targets without unwind support will abort (crash) instead of exit with failure code 101 after panicking. As a special case, allow crashes for run-fail tests for such targets.
  • Different sanitizer implementations handle detected memory problems differently. Some abort (crash) the process while others exit with failure code 1. Introduce and use run-fail-or-crash for such tests.

This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as SIGABRT in tests/ui/panics/panic-main.rs (shown as Aborted (core dumped) in the logs attached to that issue, and I have also been able to reproduce this locally).

TODO

Zulip discussion

See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235

try-job: aarch64-apple
try-job: x86_64-msvc-1
try-job: x86_64-gnu
try-job: dist-i586-gnu-i586-i686-musl
try-job: test-various
try-job: armhf-gnu

@rustbot
Copy link
Collaborator

rustbot commented Jun 25, 2025

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 25, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 25, 2025

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@Enselic Enselic added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 25, 2025
@rustbot rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Jun 25, 2025
@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

workingjubilee commented Jun 25, 2025

why not simply run-abort

also would accept run-crash

@jieyouxu
Copy link
Member

@bors2 delegate=try

@rust-bors
Copy link

rust-bors bot commented Jun 25, 2025

@Enselic can now perform try builds on this pull request

@Enselic Enselic force-pushed the tests-ui-run-fail-exit-vs-signal branch from 6ad1973 to 17be091 Compare June 25, 2025 10:12
@Enselic

This comment was marked as outdated.

@rust-bors

This comment was marked as outdated.

@Enselic
Copy link
Member Author

Enselic commented Jun 25, 2025

@bors2 try jobs=x86_64-msvc-1,x86_64-msvc-2

@rust-bors
Copy link

rust-bors bot commented Jun 25, 2025

⌛ Trying commit 17be091 with merge 873ecba

To cancel the try build, run the command @bors2 try cancel.

rust-bors bot added a commit that referenced this pull request Jun 25, 2025
…try>

tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok)

Normally a `run-fail` ui test shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement.

Some of our current tests do terminate by a signal however. Introduce and use `run-fail-without-exit-code` for those tests.

This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally).

### TODO
- [ ] what about on Windows?
- [ ] also update docs at https://rustc-dev-guide.rust-lang.org/tests/directives.html#controlling-outcome-expectations
- [ ] clean up the code

### Zulip discussion

See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235

try-job: x86_64-msvc-1
try-job: x86_64-msvc-2
@rust-bors

This comment was marked as resolved.

@Enselic Enselic force-pushed the tests-ui-run-fail-exit-vs-signal branch from 17be091 to ad4e082 Compare June 25, 2025 13:30
@Enselic
Copy link
Member Author

Enselic commented Jun 25, 2025

An only-windows test that was run-fail now must be run-crash. Fixed now. Trying again:

@bors2 try jobs=x86_64-msvc-1,x86_64-msvc-2

@rust-bors
Copy link

rust-bors bot commented Jun 25, 2025

⌛ Trying commit ad4e082 with merge e685982

To cancel the try build, run the command @bors2 try cancel.

rust-bors bot added a commit that referenced this pull request Jun 25, 2025
…try>

tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok)

Normally a `run-fail` ui test shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement.

Some of our current tests do terminate by a signal however. Introduce and use `run-fail-without-exit-code` for those tests.

This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally).

### TODO
- [ ] **Q:** what about on Windows?
    **A:** we'll treat any exit code outside of 1 - 127 as "crashed", and we'll do the same on unix.
- [ ] also update docs at https://rustc-dev-guide.rust-lang.org/tests/directives.html#controlling-outcome-expectations
- [ ] clean up the code
- [ ] test all permutations of actual vs expected

### Zulip discussion

See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235

try-job: x86_64-msvc-1
try-job: x86_64-msvc-2
@rust-bors

This comment was marked as resolved.

@Enselic Enselic force-pushed the tests-ui-run-fail-exit-vs-signal branch from ad4e082 to 2499dac Compare June 25, 2025 16:29
@rustbot rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Jun 25, 2025
@fmease
Copy link
Member

fmease commented Jul 17, 2025

I've got a CI failure in a rollup: #144067 (comment). Not sure if it's caused by this PR alone or together with other ones conglomerated. Let's rerun CI and preemptively kick this one out of the queue for now.

@bors r-

@fmease fmease closed this Jul 17, 2025
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 17, 2025
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 17, 2025
@fmease fmease reopened this Jul 17, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2025
@rust-log-analyzer

This comment has been minimized.

@fmease
Copy link
Member

fmease commented Jul 17, 2025

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 2025
…t ok)

And introduce two new directives for ui tests:
* `run-crash`
* `run-fail-or-crash`

Normally a `run-fail` ui test like tests that panic shall not be
terminated by a signal like `SIGABRT`. So begin having that as a hard
requirement.

Some of our current tests do terminate by a signal/crash however.
Introduce and use `run-crash` for those tests. Note that Windows crashes
are not handled by signals but by certain high bits set on the process
exit code. Example exit code for crash on Windows: `0xc000001d`.
Because of this, we define "crash" on all platforms as "not exit with
success and not exit with a regular failure code in the range 1..=127".

Some tests behave differently on different targets:
* Targets without unwind support will abort (crash) instead of exit with
  failure code 101 after panicking. As a special case, allow crashes for
  `run-fail` tests for such targets.
* Different sanitizer implementations handle detected memory problems
  differently. Some abort (crash) the process while others exit with
  failure code 1. Introduce and use `run-fail-or-crash` for such tests.
@Enselic Enselic force-pushed the tests-ui-run-fail-exit-vs-signal branch from f120fbe to e1d4f2a Compare July 19, 2025 16:56
@Enselic
Copy link
Member Author

Enselic commented Jul 19, 2025

Rebased on #142304 and #144073 and the rest of master. The CI failure was because this PR was combined with a new run-fail test that should have used run-crash (but it didn't know), namely the above test. So we just had some bad luck.

I'll r this again after PR CI is green.

click to expand diff of diff between last approved commit and the latest commit
diff -u <(git show f120fbe) <(git show e1d4f2a0c29)
--- /proc/self/fd/15    2025-07-19 18:56:28.381374122 +0200
+++ /proc/self/fd/18    2025-07-19 18:56:28.381374122 +0200
@@ -1,4 +1,4 @@
-commit f120fbe5c1b42dd08233ecf64eb45bc937319412 (origin/tests-ui-run-fail-exit-vs-signal-21, origin/tests-ui-run-fail-exit-vs-signal)
+commit e1d4f2a0c297690ddfc24815de57539f532f2471 (HEAD -> tests-ui-run-fail-exit-vs-signal-23, origin/tests-ui-run-fail-exit-vs-signal-23)
 Author: Martin Nordholts <[email protected]>
 Date:   Wed Jun 25 07:56:40 2025 +0200

@@ -28,7 +28,7 @@
       failure code 1. Introduce and use `run-fail-or-crash` for such tests.

 diff --git a/src/doc/rustc-dev-guide/src/tests/directives.md b/src/doc/rustc-dev-guide/src/tests/directives.md
-index 839076b809d..7c27e6a2052 100644
+index 63aa08c389c..6685add461e 100644
 --- a/src/doc/rustc-dev-guide/src/tests/directives.md
 +++ b/src/doc/rustc-dev-guide/src/tests/directives.md
 @@ -75,8 +75,10 @@ expectations](ui.md#controlling-passfail-expectations).
@@ -670,6 +670,49 @@
  //@ exec-env:RUST_BACKTRACE=0
  //@ check-run-results
  //@ error-pattern: panicked while processing panic
+diff --git a/tests/ui/panics/panic-main.rs b/tests/ui/panics/panic-main.rs
+index 2395f68241f..2009f69e19e 100644
+--- a/tests/ui/panics/panic-main.rs
++++ b/tests/ui/panics/panic-main.rs
+@@ -1,30 +1,37 @@
+ //@ revisions: default abort-zero abort-one abort-full unwind-zero unwind-one unwind-full
+
++//@[default] run-fail
++
+ //@[abort-zero] compile-flags: -Cpanic=abort
+ //@[abort-zero] no-prefer-dynamic
+ //@[abort-zero] exec-env:RUST_BACKTRACE=0
++//@[abort-zero] run-crash
+
+ //@[abort-one] compile-flags: -Cpanic=abort
+ //@[abort-one] no-prefer-dynamic
+ //@[abort-one] exec-env:RUST_BACKTRACE=1
++//@[abort-one] run-crash
+
+ //@[abort-full] compile-flags: -Cpanic=abort
+ //@[abort-full] no-prefer-dynamic
+ //@[abort-full] exec-env:RUST_BACKTRACE=full
++//@[abort-full] run-crash
+
+ //@[unwind-zero] compile-flags: -Cpanic=unwind
+ //@[unwind-zero] exec-env:RUST_BACKTRACE=0
+ //@[unwind-zero] needs-unwind
++//@[unwind-zero] run-fail
+
+ //@[unwind-one] compile-flags: -Cpanic=unwind
+ //@[unwind-one] exec-env:RUST_BACKTRACE=1
+ //@[unwind-one] needs-unwind
++//@[unwind-one] run-fail
+
+ //@[unwind-full] compile-flags: -Cpanic=unwind
+ //@[unwind-full] exec-env:RUST_BACKTRACE=full
+ //@[unwind-full] needs-unwind
++//@[unwind-full] run-fail
+
+-//@ run-fail
+ //@ error-pattern:moop
+ //@ needs-subprocess
+
 diff --git a/tests/ui/precondition-checks/alignment.rs b/tests/ui/precondition-checks/alignment.rs
 index 92400528fa0..038a625bed7 100644
 --- a/tests/ui/precondition-checks/alignment.rs

@Enselic
Copy link
Member Author

Enselic commented Jul 19, 2025

Hmm the spelling mistake caught by the Spellcheck CI job is not introduced by me as far as I can tell. Probably a temporary problem during a transition period. Hopefully bors ignores that job for now.

@fmease
Copy link
Member

fmease commented Jul 19, 2025

Thanks! @bors r=petrochenkov

Hmm the spelling mistake caught by the Spellcheck CI job is not introduced by me as far as I can tell. Probably a temporary problem during a transition period. Hopefully bors ignores that job for now.

Yeah, see #t-infra > Spellcheck workflow now fails on all PRs (tree bad?) @ 💬. Indeed, this doesn't prevent bors merges / won't fail bors's auto builds, so we can ignore it.

@bors
Copy link
Collaborator

bors commented Jul 19, 2025

📌 Commit e1d4f2a has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 19, 2025
@bors
Copy link
Collaborator

bors commented Jul 20, 2025

⌛ Testing commit e1d4f2a with merge 6707bf0...

@bors
Copy link
Collaborator

bors commented Jul 20, 2025

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 6707bf0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 20, 2025
@bors bors merged commit 6707bf0 into rust-lang:master Jul 20, 2025
11 of 12 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 20, 2025
Copy link
Contributor

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing ee3a078 (parent) -> 6707bf0 (this PR)

Test differences

Show 2 test diffs

2 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 6707bf0f59485cf054ac1095725df43220e4be20 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. pr-check-2: 2126.3s -> 3276.3s (54.1%)
  2. x86_64-apple-2: 3681.7s -> 5300.6s (44.0%)
  3. dist-x86_64-solaris: 5066.8s -> 6080.1s (20.0%)
  4. dist-apple-various: 6246.5s -> 7299.6s (16.9%)
  5. x86_64-rust-for-linux: 2529.5s -> 2936.6s (16.1%)
  6. x86_64-apple-1: 8162.5s -> 9374.9s (14.9%)
  7. pr-check-1: 1516.3s -> 1696.7s (11.9%)
  8. i686-gnu-1: 7159.4s -> 7988.9s (11.6%)
  9. x86_64-gnu-tools: 3317.2s -> 3699.9s (11.5%)
  10. aarch64-gnu-debug: 3851.7s -> 4263.9s (10.7%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6707bf0): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

  • If the regression was expected or you think it can be justified,
    please write a comment with sufficient written justification, and add
    @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
  • If you think that you know of a way to resolve the regression, try to create
    a new PR with a fix for the regression.
  • If you do not understand the regression or you think that it is just noise,
    you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
    were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
0.3% [0.3%, 0.3%] 1
Regressions ❌
(secondary)
0.5% [0.4%, 0.8%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.3% [0.3%, 0.3%] 1

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (secondary -3.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.0% [-3.0%, -3.0%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 464.521s -> 465.018s (0.11%)
Artifact size: 374.65 MiB -> 374.58 MiB (-0.02%)

@rustbot rustbot added the perf-regression Performance regression. label Jul 20, 2025
github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Jul 21, 2025
…etrochenkov

tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok)

And introduce two new directives for ui tests:
* `run-crash`
* `run-fail-or-crash`

Normally a `run-fail` ui test like tests that panic shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement.

Some of our current tests do terminate by a signal/crash however.  Introduce and use `run-crash` for those tests. Note that Windows crashes are not handled by signals but by certain high bits set on the process  exit code. Example exit code for crash on Windows: `0xc000001d` (`STATUS_ILLEGAL_INSTRUCTION`).  Because of this, we define "crash" on all platforms as "not exit with success and not exit with a regular failure code in the range 1..=127".

Some tests behave differently on different targets:
* Targets without unwind support will abort (crash) instead of exit with   failure code 101 after panicking. As a special case, allow crashes for   `run-fail` tests for such targets.
* Different sanitizer implementations handle detected memory problems   differently. Some abort (crash) the process while others exit with   failure code 1. Introduce and use `run-fail-or-crash` for such tests.

This adds further (cc rust-lang/rust#142304, rust-lang/rust#142886) protection against the regression in rust-lang/rust#123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally).

### TODO
- [x] **Q:** what about on Windows?
    **A:** we'll treat any exit code outside of 1 - 127 as "crashed", and we'll do the same on unix.
- [x] test all permutations of actual vs expected
    **Done:** See rust-lang/rust#143002 (comment).
- [x] Handle targets without unwind support
- [x] Add `run-fail-or-crash` for some sanitizer tests
- [x] remote-test-client. See rust-lang/rust#143448

### Zulip discussion

See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235

try-job: aarch64-apple
try-job: x86_64-msvc-1
try-job: x86_64-gnu
try-job: dist-i586-gnu-i586-i686-musl
try-job: test-various
try-job: armhf-gnu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-rustc-dev-guide Area: rustc-dev-guide A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.